Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP update Stan to lvl6 #1

Open
wants to merge 20 commits into
base: develop
Choose a base branch
from
Open

WIP update Stan to lvl6 #1

wants to merge 20 commits into from

Conversation

iskelic
Copy link

@iskelic iskelic commented Mar 29, 2021

Description:

Updating PHPStan to lvl6

@iskelic iskelic requested a review from penovic March 29, 2021 09:43
@iskelic iskelic self-assigned this Mar 29, 2021
@KondukterCRO KondukterCRO force-pushed the refactor/update-phpstan branch from 8b1baff to c5870e3 Compare February 9, 2022 17:18
@ghost
Copy link

ghost commented Mar 22, 2022

@penovic @iskelic I tried this one out, phpstan is all green, is there any further work on this one, perhaps adding psalm?

@KondukterCRO KondukterCRO changed the base branch from master to develop May 3, 2023 12:26
@ghost ghost requested review from a user and KondukterCRO and removed request for penovic October 25, 2023 11:43
/**
* @return array<string,mixed>
*
* @psalm-suppress PossiblyUnusedMethod
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mplesa1 this method is used inside the project, how come Psalm recognizes it as unused?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Used inside tests

*
* @throws JsonApiEncodingException
*
* @psalm-suppress PossiblyUnusedMethod
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Used inside tests?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even though it's used, Psalm recognizes it as unused?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed the annotation because it's also used inside PhpArrayToRelationshipCollectionEncoder. But yes, if it's only used inside some test we need annotation.

@mplesa1
Copy link

mplesa1 commented Oct 25, 2023

@penovic @iskelic I tried this one out, phpstan is all green, is there any further work on this one, perhaps adding psalm?

psalm is added

@@ -6,7 +6,9 @@

use Undabot\JsonApi\Definition\Model\Resource\ResourceInterface;

/** @psalm-suppress UnusedClass */
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Psalm documentation says the following:

If this class is used and part of the public API, annotate it with @psalm-api.

I believe we should check all of the classes that are used as part of json-api-symfony and mark them with @psalm-api instead of suppressing the error.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch, I replace it

$rawMeta = $resource['meta'] ?? null;
$rawLink = $resource['links']['self'] ?? null;

$rawAttributes = $this->assertStringKeyArray($resource['attributes'] ?? []);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about adding a string $key to this method, resulting in the coalescing operator being moved to the assertStringKeyArray method to avoid doing it outside on each call to the method?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agree, good catch, I refactored both assertStringKeyArray and assertStringKeyArrayNested

Comment on lines 96 to 116
private function assertStringKeyArray(mixed $array): array
{
if (!is_array($array)) {
throw new \InvalidArgumentException("Parameter is not array.");
}
foreach ($array as $key) {
if (!is_string($key)) {
throw new \InvalidArgumentException("Array key must be a string.");
}
}

return $array;

}

/**
*
* @param mixed $array
* @return array<string, array<string, mixed>>
*/
function assertStringKeyArrayNested(mixed $array): array
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These methods could be moved to some sort of a utility class if we're going to need them in more places?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I move them to the existing abstract class ArrayUtil

/**
* @implements IteratorAggregate<int,Sort>
*/
class SortSet implements \IteratorAggregate
{
/** @var Sort[] */
private $sorts;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

array?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

class AttributeCollection implements AttributeCollectionInterface
{
/** @var Attribute[] */
/** @var AttributeInterface[] */
private $attributes;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

array?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

use Undabot\JsonApi\Definition\Model\Resource\Relationship\RelationshipCollectionInterface;
use Undabot\JsonApi\Definition\Model\Resource\Relationship\RelationshipInterface;

class RelationshipCollection implements RelationshipCollectionInterface
{
/** @var array */
/** @var RelationshipInterface[] */
private $relationships = [];
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

array?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Comment on lines +28 to +30
// $message = sprintf('ResourceIdentifierInterface expected, %s given', \get_class($resourceIdentifier));
//
// throw new InvalidArgumentException($message);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this commented out?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know, @KondukterCRO did those changes in this branch d05ac8b#diff-390909c32070ee11ae2baba70ca5c9e766a3b2bf1a3a2297dd63ddde7a79f4db Pena, do you have some information?

Comment on lines +24 to +26
// $message = sprintf('ResourceIdentifierInterface expected, %s given', \get_class($resourceIdentifier));
//
// throw new InvalidArgumentException($message);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same question as before

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@KondukterCRO same question as before

@@ -8,5 +8,8 @@

interface LinkToPhpArrayEncoderInterface
{
/**
* @return null|array<string,mixed>|string
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😱

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants